Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5892faaf2a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }.onSuccess { | ||
| scope.launch { |
There was a problem hiding this comment.
sendDraftUseCase reports per-account failures via ComposeProgressState.Error and returns normally, so this unconditional onSuccess branch still emits ComposeProgressState.Success afterward. In multi-account sends where one or more accounts fail, the UI gets an error and then a success signal for the same action, which can incorrectly show a successful compose notification/state despite failed deliveries.
Useful? React with 👍 / 👎.
| } | ||
| progressTracker.onComposeSuccess(target.account.accountKey) | ||
| progress(progressTracker.state()) | ||
| draftRepository.deleteTarget(groupId, target.account.accountKey) |
There was a problem hiding this comment.
After a target succeeds, only the DB target row is removed here; no path calls DraftMediaStore.delete when the final target for a group succeeds. That leaves files under draft_media/<groupId>/... orphaned after every successful post with attachments, so local storage usage grows over time with no references left in the database.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Surface failures returned by tryRun in compose dispatch
tryRun wraps this send path and returns a Result, but the result is ignored here. If sendDraftUseCase throws before it can emit ComposeProgressState.Error (for example, a media read or draft persistence exception), the exception is swallowed and no terminal state is sent, so callers can be left at the initial progress state without any failure notification even though nothing was dispatched.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| accountType = args.accountType, | ||
| toCompose = { | ||
| navigate(Route.Compose.New(args.accountType)) | ||
| navigate(Route.Compose.New) |
There was a problem hiding this comment.
This navigation no longer carries args.accountType, so Route.Compose.New opens compose without account context. In the new flow, ComposeEntryBuilder passes accountType = null, and ComposePresenter falls back to the active account; when Home is showing a different account, tapping compose can default to the wrong sender account and lead to accidental posts from that account.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR introduces cross-platform draft persistence + a “Draft Box” UI, refactors composing to support multi-account sending via persisted drafts, and improves reference cleanup / VVO delete behavior.
Changes:
- Add draft persistence primitives (Room schema + repository + media store) and draft send/restore/save use cases.
- Add Draft Box UI/presenter and wire navigation from Settings/Compose across Android/Desktop/iOS.
- Refactor compose pipeline (ComposeData/compose progress, path producing) to support draft-backed sending and platform-specific media path handling.
Reviewed changes
Copilot reviewed 94 out of 96 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| shared/src/jvmTest/kotlin/dev/dimension/flare/TestFileHelper.jvm.kt | JVM test utilities for temporary root + file items. |
| shared/src/jvmTest/kotlin/dev/dimension/flare/DatabaseHelper.jvm.kt | JVM test DB builder now routes by KClass for supported DBs. |
| shared/src/jvmMain/kotlin/dev/dimension/flare/di/PlatformModule.jvm.kt | Bind JVM PlatformPathProducer implementation into DI. |
| shared/src/jvmMain/kotlin/dev/dimension/flare/data/repository/DraftMediaStore.jvm.kt | JVM actual for building FileItem from persisted draft media paths. |
| shared/src/jvmMain/kotlin/dev/dimension/flare/data/io/PlatformPathProducer.jvm.kt | Remove old JVM expect/actual class implementation. |
| shared/src/jvmMain/kotlin/dev/dimension/flare/data/io/JvmPlatformPathProducer.kt | New JVM PlatformPathProducer implementation (datastore + draft media paths). |
| shared/src/jvmMain/kotlin/dev/dimension/flare/common/FileItem.jvm.kt | JVM FileItem type detection refactor (extension-based). |
| shared/src/commonTest/kotlin/dev/dimension/flare/ui/route/DeeplinkRouteTest.kt | Update deeplink compose-new test for data object route. |
| shared/src/commonTest/kotlin/dev/dimension/flare/ui/presenter/compose/InitialTextResolverTest.kt | New tests for initial-text resolution logic (reply mentions / VVO quote). |
| shared/src/commonTest/kotlin/dev/dimension/flare/TestFileHelper.kt | Add expect/actual test file helpers for draft media tests. |
| shared/src/commonTest/kotlin/dev/dimension/flare/DatabaseHelper.kt | Adjust expect/actual test DB builder to KClass + keep reified helper. |
| shared/src/commonTest/kotlin/dev/dimension/flare/data/repository/DraftRepositoryTest.kt | New tests for draft repository persistence + deletion semantics. |
| shared/src/commonTest/kotlin/dev/dimension/flare/data/repository/DraftMediaStoreTest.kt | New tests for persist/restore/delete media flow + filename sanitization. |
| shared/src/commonTest/kotlin/dev/dimension/flare/data/datasource/microblog/handler/PostHandlerTest.kt | Update paging assertions after delete behavior change. |
| shared/src/commonTest/kotlin/dev/dimension/flare/data/database/cache/mapper/MicroblogTest.kt | Rename/add tests for reference deletion semantics (reply vs quote). |
| shared/src/commonMain/kotlin/dev/dimension/flare/ui/route/DeeplinkRoute.kt | Compose.New becomes data object (no accountType payload). |
| shared/src/commonMain/kotlin/dev/dimension/flare/ui/presenter/status/action/AddReactionPresenter.kt | Pass accountType into EmojiData for emoji history scoping. |
| shared/src/commonMain/kotlin/dev/dimension/flare/ui/presenter/home/AllNotificationPresenter.kt | Move from CacheState-based wiring to UiState mapping helpers. |
| shared/src/commonMain/kotlin/dev/dimension/flare/ui/presenter/compose/SendDraftUseCase.kt | New use case to persist then send drafts across multiple accounts with progress tracking. |
| shared/src/commonMain/kotlin/dev/dimension/flare/ui/presenter/compose/SaveDraftUseCase.kt | New use case to persist a draft (content + targets + medias). |
| shared/src/commonMain/kotlin/dev/dimension/flare/ui/presenter/compose/RestoreDraftUseCase.kt | New use case to load draft into UI model + ComposeData. |
| shared/src/commonMain/kotlin/dev/dimension/flare/ui/presenter/compose/InitialTextResolver.kt | New initial text resolver for replies/quotes across platforms. |
| shared/src/commonMain/kotlin/dev/dimension/flare/ui/presenter/compose/DraftBoxPresenter.kt | New presenter to list/send/retry/delete drafts for Draft Box UI. |
| shared/src/commonMain/kotlin/dev/dimension/flare/ui/presenter/compose/ComposeUseCase.kt | Compose now uses draft-backed save/send (multi-account) + groupId support. |
| shared/src/commonMain/kotlin/dev/dimension/flare/ui/model/UiEmoji.kt | EmojiData now carries AccountType. |
| shared/src/commonMain/kotlin/dev/dimension/flare/ui/model/UiDraft.kt | New UI model for drafts (accounts, medias, status, updatedAt). |
| shared/src/commonMain/kotlin/dev/dimension/flare/di/CommonModule.kt | Register DraftRepository/DraftMediaStore and draft use cases in Koin. |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/repository/DraftRepository.kt | New repository: store draft groups/targets/medias + flows and lifecycle ops. |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/repository/DraftMediaStore.kt | New persisted media store for drafts + filename sanitization and cleanup. |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/repository/AccountRepository.kt | Add find(accountKey) helper for draft resend resolution. |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/network/vvo/api/StatusApi.kt | Add VVO deleteComment endpoint. |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/io/PlatformPathProducer.kt | Replace expect class with common interface incl. draftMediaFile(). |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/datasource/xqt/XQTDataSource.kt | Compose progress callback signature change + quote resolution adjustment. |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/datasource/vvo/VVOLoader.kt | Try deleting as status, then fall back to deleting as comment. |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/datasource/vvo/VVODataSource.kt | Compose progress callback signature change + emoji config includes accountKey. |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/datasource/misskey/MisskeyDataSource.kt | Compose progress callback signature change + emoji config includes accountKey. |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/datasource/microblog/handler/PostHandler.kt | Adjust DB delete ordering/arguments during status deletion. |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/datasource/microblog/ComposeProgress.kt | Remove old ComposeProgress model. |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/datasource/microblog/ComposeData.kt | Remove embedded account + referenceStatus.data from ComposeData. |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/datasource/microblog/ComposeConfig.kt | Emoji config now includes accountKey and preserves it on merge. |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/datasource/microblog/AuthenticatedMicroblogDataSource.kt | Compose progress callback now () -> Unit. |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/datasource/mastodon/MastodonDataSource.kt | Compose progress callback signature change + emoji config includes accountKey. |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/datasource/bluesky/BlueskyDataSource.kt | Compose progress callback signature change + repo id source fix. |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/database/cache/mapper/Microblog.kt | Implement reference cleanup (remove stale quote/etc, conditional reply cleanup). |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/database/cache/dao/StatusReferenceDao.kt | Add delete-by-(keys, referenceTypes) query. |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/database/app/model/DbDraft.kt | Add draft Room entities + converters (content/target/media types). |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/database/app/dao/DraftDao.kt | Add DraftDao queries for visibility/sending lists + status updates/expiry reset. |
| shared/src/commonMain/kotlin/dev/dimension/flare/data/database/app/AppDatabase.kt | Add draft entities/dao/converters, bump schema to v6 + auto-migration. |
| shared/src/commonMain/kotlin/dev/dimension/flare/common/ImmutableListWrapper.kt | Move combineLatestFlowLists out to FlowExt. |
| shared/src/commonMain/kotlin/dev/dimension/flare/common/FlowExt.kt | New Flow extension with combineLatestFlowLists. |
| shared/src/appleTest/kotlin/dev/dimension/flare/TestFileHelper.apple.kt | Apple test utilities for temp root + file items. |
| shared/src/appleTest/kotlin/dev/dimension/flare/DatabaseHelper.apple.kt | Apple test DB builder updated to KClass routing. |
| shared/src/appleTest/kotlin/dev/dimension/flare/common/SerializationFormatBenchmarkAppleTest.kt | Add Apple-side serialization benchmark test. |
| shared/src/appleMain/kotlin/dev/dimension/flare/di/PlatformModule.apple.kt | Bind Apple PlatformPathProducer implementation into DI. |
| shared/src/appleMain/kotlin/dev/dimension/flare/data/repository/DraftMediaStore.apple.kt | Apple actual for building FileItem from persisted draft media paths. |
| shared/src/appleMain/kotlin/dev/dimension/flare/data/io/ApplePlatformPathProducer.kt | New Apple PlatformPathProducer implementation with draftMediaFile(). |
| shared/src/appleMain/kotlin/dev/dimension/flare/common/FileItem.apple.kt | Apple FileItem now supports lazy loading from path (for drafts). |
| shared/src/androidMain/kotlin/dev/dimension/flare/di/PlatformModule.android.kt | Bind Android PlatformPathProducer implementation into DI. |
| shared/src/androidMain/kotlin/dev/dimension/flare/data/repository/DraftMediaStore.android.kt | Android actual for building FileItem from persisted draft media paths. |
| shared/src/androidMain/kotlin/dev/dimension/flare/data/io/PlatformPathProducer.android.kt | Remove old Android expect/actual class implementation. |
| shared/src/androidMain/kotlin/dev/dimension/flare/data/io/AndroidPlatformPathProducer.kt | New Android PlatformPathProducer implementation with draftMediaFile(). |
| shared/src/androidMain/kotlin/dev/dimension/flare/common/FileItem.android.kt | Android FileItem refactor: support UriSource and PathSource for drafts. |
| shared/src/androidHostTest/kotlin/dev/dimension/flare/DatabaseHelper.android.kt | Android host test DB builder updated to accept KClass databaseClass. |
| shared/schemas/dev.dimension.flare.data.database.app.AppDatabase/6.json | Add Room schema snapshot for v6. |
| iosApp/flare/UI/Screen/StatusAddReactionSheet.swift | EmojiPopup init updated to use EmojiData.accountType. |
| iosApp/flare/UI/Screen/SettingsScreen.swift | Add navigation entry to Draft Box. |
| iosApp/flare/UI/Screen/DraftBoxScreen.swift | New Draft Box screen UI on iOS. |
| iosApp/flare/UI/Screen/ComposeScreen.swift | Compose: draft save prompt, load/apply draft, open Draft Box sheet, multi-account selection wiring changes. |
| iosApp/flare/UI/Route/Router.swift | Route switch updated to include composeDraft. |
| iosApp/flare/UI/Route/Route.swift | Add DraftBox/ComposeDraft routes and deeplink mapping updates. |
| iosApp/flare/UI/Component/EmojiPopup.swift | EmojiPopup uses EmojiData.accountType for history presenter. |
| iosApp/flare/Assets.xcassets/fa-inbox.symbolset/Contents.json | Add Draft Box empty-state icon asset metadata. |
| gradle.properties | Enable Kotlin incremental native compilation; remove linuxX64 cacheKind override. |
| desktopApp/src/main/kotlin/dev/dimension/flare/ui/screen/settings/SettingsScreen.kt | Add Draft Box entry and plumb toDraftBox navigation callback. |
| desktopApp/src/main/kotlin/dev/dimension/flare/ui/screen/compose/DraftBoxScreen.kt | New Draft Box screen on Desktop (Compose Multiplatform). |
| desktopApp/src/main/kotlin/dev/dimension/flare/ui/screen/compose/ComposeDialog.kt | Compose: draft prompt on close, open Draft Box, apply loaded draft, multi-account selection updates. |
| desktopApp/src/main/kotlin/dev/dimension/flare/ui/route/Router.kt | Add DraftBox route and Compose.Draft floating route wiring. |
| desktopApp/src/main/kotlin/dev/dimension/flare/ui/route/Route.kt | Add DraftBox + Compose.Draft routes; Compose.New becomes object. |
| desktopApp/src/main/kotlin/dev/dimension/flare/common/FlareHardwareShortcutDetector.kt | Round bounds comparisons to avoid float precision edge cases in pan detection. |
| desktopApp/src/main/kotlin/dev/dimension/flare/App.kt | Update compose navigation to Route.Compose.New object. |
| desktopApp/src/main/composeResources/values/strings.xml | Add draft-related strings + close-confirm strings. |
| app/src/main/res/values/strings.xml | Add draft-related strings + close-confirm strings. |
| app/src/main/java/dev/dimension/flare/ui/screen/settings/SettingsSelectEntryBuilder.kt | Wire navigation from settings entry builder to DraftBox route. |
| app/src/main/java/dev/dimension/flare/ui/screen/settings/SettingsScreen.kt | Add Draft Box entry in Android settings UI. |
| app/src/main/java/dev/dimension/flare/ui/screen/home/HomeScreen.kt | Route.Compose.New becomes object; simplify FAB conditions and user state usage. |
| app/src/main/java/dev/dimension/flare/ui/screen/home/HomeEntryBuilder.kt | Update timeline compose navigation to Route.Compose.New object. |
| app/src/main/java/dev/dimension/flare/ui/screen/compose/DraftBoxScreen.kt | New Draft Box screen on Android. |
| app/src/main/java/dev/dimension/flare/ui/screen/compose/ComposeScreen.kt | Compose: draft prompt on close, open Draft Box, apply loaded draft, multi-account selection updates. |
| app/src/main/java/dev/dimension/flare/ui/screen/compose/ComposeEntryBuilder.kt | Add DraftBox and Compose.Draft routes; Compose.New uses null accountType + draft box entry points. |
| app/src/main/java/dev/dimension/flare/ui/route/Route.kt | Add DraftBox and Compose.Draft routes; Compose.New becomes object; deeplink mapping update. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }.map { | ||
| remember(it) { | ||
| EmojiData(it) | ||
| EmojiData(it, accountType) | ||
| } |
There was a problem hiding this comment.
EmojiData now captures accountType, but this remember is keyed only by it (the emoji map). If accountType changes while the emoji map instance stays the same, Compose will keep the old EmojiData with a stale accountType, which can break emoji history/scoping. Key the remember by both it and accountType (or avoid remember here).
| val state = state | ||
| val showAccountSelectMenu = showAccountSelectMenu | ||
| val languageState = languageState | ||
| val hasTextContent = textFieldState.text.toString().isNotBlank() |
There was a problem hiding this comment.
The close-confirm prompt is gated by state.hasTextContent, but in this presenter hasTextContent only checks the main text field. If the user has only media, CW text, or a poll configured, they can still lose changes without any prompt. Consider broadening this to an hasUnsavedContent check that also includes media count, CW enabled/non-empty, and poll enabled (and any other relevant fields).
| val hasTextContent = textFieldState.text.toString().isNotBlank() | |
| val hasTextContent = | |
| textFieldState.text.toString().isNotBlank() || | |
| mediaState.takeSuccess()?.medias.orEmpty().isNotEmpty() || | |
| (contentWarningState | |
| .takeSuccess() | |
| ?.textFieldState | |
| ?.text | |
| ?.toString() | |
| ?.isNotBlank() == true) || | |
| (pollState.takeSuccess()?.enabled == true) |
| onBack() | ||
| }, | ||
| ) { | ||
| Text(text = stringResource(android.R.string.cancel)) |
There was a problem hiding this comment.
The dialog dismiss button uses the system "Cancel" label (android.R.string.cancel) but the handler actually discards the compose content by calling onBack(). This is misleading for users. Use the dedicated discard string (you added compose_close_discard) and reserve "Cancel" for closing the dialog and staying on the compose screen.
| Text(text = stringResource(android.R.string.cancel)) | |
| Text(text = stringResource(id = R.string.compose_close_discard)) |
| fun closeOrConfirm() { | ||
| if (state.hasTextContent) { | ||
| showCloseConfirmDialog = true | ||
| } else { | ||
| onBack?.invoke() | ||
| } |
There was a problem hiding this comment.
closeOrConfirm() uses state.hasTextContent, but in this file hasTextContent is derived only from the main text field. Users can attach media / add a poll / set content warning without any text, and currently they can close the dialog without being prompted to save a draft. Consider switching this to an hasUnsavedContent check that also includes media/poll/CW state.
| primaryButtonText = stringResource(Res.string.media_save), | ||
| secondaryButtonText = stringResource(Res.string.cancel), | ||
| onButtonClick = { | ||
| when (it) { | ||
| ContentDialogButton.Primary -> { | ||
| state.saveDraft { dispatched -> | ||
| if (dispatched) { | ||
| showCloseConfirmDialog = false | ||
| onBack?.invoke() | ||
| } | ||
| } | ||
| } | ||
| ContentDialogButton.Secondary -> { | ||
| showCloseConfirmDialog = false | ||
| onBack?.invoke() | ||
| } |
There was a problem hiding this comment.
In the close confirmation dialog, the secondary button text is "Cancel" but clicking it actually closes the composer (onBack?.invoke()) and discards changes. This is confusing UX. Use a discard label (there is a compose_close_discard string) for the destructive/close path, and make "Cancel" simply dismiss the dialog and keep editing.
| draftRepository.updateTargetStatus( | ||
| groupId = groupId, | ||
| accountKey = target.account.accountKey, | ||
| status = DraftTargetStatus.SENDING, | ||
| attemptCount = 1, | ||
| lastAttemptAt = Clock.System.now().toEpochMilliseconds(), | ||
| ) |
There was a problem hiding this comment.
attemptCount is always set to 1 when starting a send, even when re-sending an existing draft target. This throws away prior attempt history and can break any retry/backoff/expiry logic based on attempt count. Consider reading the current attempt count for the target and incrementing it (or passing it through from the draft targets).
| draftRepository.updateTargetStatus( | ||
| groupId = groupId, | ||
| accountKey = target.account.accountKey, | ||
| status = DraftTargetStatus.FAILED, | ||
| errorMessage = throwable.message, | ||
| attemptCount = 1, | ||
| lastAttemptAt = Clock.System.now().toEpochMilliseconds(), | ||
| ) |
There was a problem hiding this comment.
On failure, attemptCount is again hard-coded to 1, which overwrites any existing attempt count for that target. This makes it impossible to track repeated failures across retries. Increment (or at least preserve) the existing attempt count when updating to FAILED.
| if (ok == 0L) { | ||
| val response = | ||
| service.deleteComment( | ||
| cid = statusKey.id, | ||
| st = st, | ||
| ) | ||
| val ok = response.ok ?: 0 | ||
| if (ok == 0L) { | ||
| throw Exception("failed to delete status") | ||
| } |
There was a problem hiding this comment.
The thrown exception message here is very generic ("failed to delete status") and loses important context like which endpoint(s) failed and the response payload. Consider including the statusKey, and distinguishing between status-delete vs comment-delete failures (and/or surfacing the server error message/code if available).
| public actual class FileItem( | ||
| private val file: File, | ||
| internal actual val name: String? = file.name, | ||
| internal actual val type: FileType = resolveType(file.name), | ||
| ) { |
There was a problem hiding this comment.
FileItem.type on JVM is now resolved purely from the file name extension. This is a behavior regression from the previous MIME-type probing and can misclassify images/videos when the file has no extension or a misleading extension. Consider restoring a MIME probe fallback (e.g., Files.probeContentType) before falling back to extension heuristics.
| if (post.quote.any() && composeStatus is ComposeStatus.Quote) { | ||
| return InitialText( | ||
| text = "//@${post.user?.name?.raw}:${post.content.raw}", | ||
| cursorPosition = 0, | ||
| ) |
There was a problem hiding this comment.
post.user is nullable, but the VVo quote text interpolates post.user?.name?.raw directly. When user (or name) is null this will literally render "null" into the compose text (e.g., //@null:...). Guard on a non-null/non-blank author name (or fall back to handle) before constructing the quote prefix.
fix #1857